[Feature] include/exclude filters (#1) #2

rnsc · 2021-06-28T14:39:45Z

Here's my attempt at the include/exclude filtering following issue #1.
I've picked up the suggestion regarding the use of regex from @micalevisk.

Disclaimer, I'm no programmer, so I hope I haven't made any terrible mistakes.

* First commit with include/exclude filters * Enable manual workflow runs * Updated index.js * add an action run with filters * required include * negative lookup * updated index.js * updated package.json * new regex test * updated workflow * rollback packages update * change the invert match logic * fix regex * testing dummy files * better assignment * remove dummy test files * original packages.json * fix yarn issue

micalevisk

well done!

rnsc · 2021-06-30T16:51:47Z

@micalevisk @Ana06 do you think we should already merge this as is and review the suggestion on the issue #1 first or rework this completely?

micalevisk · 2021-06-30T17:12:08Z

@micalevisk @Ana06 do you think we should already merge this as is and review the suggestion on the issue #1 first or rework this completely?

I like that suggestion because it seems like more idiomatic to GH Actions

with:
  paths:
    - '*.yml'
    - '!blah/test/**'

To implement this, I'd say is better to open a new PR and close this one.

Ana06

Great work @rnsc! Thank you so much for sending a PR ❤️ and sorry for the late review, busy week! 😅
Some documentation about this great new features on the README would be nice 😉

Thanks @micalevisk for reviewing the PR and for sharing your opinion! 😄

I really do not have an strong opinion about what option we should take (GH Actions like vs include/exclude). As already two persons mentioned they prefer the GH Actions like syntax I tend to think we should use that one. @rnsc do you want to work on that or do you prefer we merge this (maybe after addressing the minor changes and adding documentation if you have time) and then someone else sends a PR for the GH Actions like implementation?

.github/workflows/test.yml

Ana06 · 2021-06-30T18:03:31Z

.github/workflows/test.yml


-      - name: Print steps context output
+      - name: Print files


[nitpick] I think the name was accurate, but it is not very important. Does someone else has an opinion about this?

I wanted to have a different name since I had duplicated the step with the exclude filter.

dist/index.js

rnsc · 2021-06-30T18:49:01Z

Great work @rnsc! Thank you so much for sending a PR ❤️ and sorry for the late review, busy week! 😅
Some documentation about this great new features on the README would be nice 😉

Thanks @micalevisk for reviewing the PR and for sharing your opinion! 😄

I really do not have an strong opinion about what option we should take (GH Actions like vs include/exclude). As already two persons mentioned they prefer the GH Actions like syntax I tend to think we should use that one. @rnsc do you want to work on that or do you prefer we merge this (maybe after addressing the minor changes and adding documentation if you have time) and then someone else sends a PR for the GH Actions like implementation?

Hey @Ana06 Thank you for taking the time to review this, don't feel pressured to do so if you have other stuff on your hands though, I was merely mentioning you for visibility regarding the feedback I had received on the issue for the feature request itself!

Adding @agraul @micalevisk too for the following inquiry:

I'll check if I can do something about accepting a list of containing both include and exclude like suggested in this example: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-using-positive-and-negative-patterns-1

The paths definition for files is not a regex but a glob pattern though.
Do you think the use of a library like this one glob is ok?

micalevisk · 2021-06-30T20:51:11Z

Do you think the use of a library like this one glob is ok?

I think it is since it follows the filter pattern cheat sheet (you could add this cheat sheet to the README)

agraul · 2021-07-01T15:33:50Z

The paths definition for files is not a regex but a glob pattern though.

Isn't that already the case? Looking at the README and the current code, the default filter is *, which is a quantifier and doesn't match anything by itself. I don't understand how it currently works, JS is not a language I know well. I would expect .* is needed to match everything.

Do you think the use of a library like this one glob is ok?

I'm no expert, from a first glance it looks like it should work.

Support for glob filtering with multi-rules.

rnsc · 2021-07-01T20:11:43Z

Hello @Ana06 @micalevisk @agraul ,

I believe I have the correct logic in place to support glob filtering.
I've updated the README with the example.
Should I also mention the library used in the README? https://github.com/isaacs/minimatch
So that people understand how the glob pattern matching is done?

Thank you for your time and feedback on this.
Again, I'm no developer, so I know my code is probably not the best!

rnsc · 2021-07-01T20:12:47Z

README.md

@@ -1,16 +1,18 @@
+# get-changed-files


I did a bit of linting on the README too, I hope that's ok

That's awesome, thanks!

README.md

rnsc · 2021-07-01T20:13:54Z

README.md

@@ -22,16 +24,23 @@ See [action.yml](action.yml)
    # Default: 'space-delimited'
    format: ''
    # Filter files using a regex
-    filter: '*'
+    glob-filter: '*'


Same here, the verison of the action should be changed too right?

it should. yarn version --no-git-tag-version --major

right, @micalevisk, @Ana06, should we also change the repository URLs in the package.json? Or are we planning on trying to merge in the original repo in the future?

I did the yarn command thingie, I'll let you all check :)

should we call it grob-filter or just filter? I like filter, as we don't have any other filer I think the name doesn't need to be more specific as long as we document it properly. But I don't have a strong opinion... 🤔 🤔 Any other opinion?

@rnsc

should we also change the repository URLs in the package.json? Or are we planning on trying to merge in the original repo in the future?

I think we can change the urls. I was hopping upstream integrates these changes when I originally forked it, but it doesn't look like it will happen.

rnsc · 2021-07-01T20:15:01Z

package.json

@@ -39,7 +39,7 @@
    "all": "yarn clean && yarn build && yarn format && yarn lint && yarn package && yarn test"
  },
  "dependencies": {
-    "@actions/core": "^1.2.2",
+    "@actions/core": "^1.4.0",


I bumped it to a more recent version to have getMultilineInput support.

src/main.ts

package.json

rnsc · 2021-07-06T11:27:46Z

README.md

+    format: ''
+    # Filter files using a regex
+    glob-filter: |
+      *.yml


@agraul not sure if you've seen this, but as GitHub actions core doesn't support a standard YAML as input, we have to declare the list a multi line string.

Thanks for the ping, I missed that. It's a bit unfortunate, I didn't know that lists can't be passed, but as long as the structure is kept I think this is still nice!

rnsc · 2021-07-12T07:31:49Z

Hello @Ana06
Do you think you'll have time this week ro review this PR again?

Thanks!

Ana06

Awesome work! Just some minor comments. Once those are addressed/discussed, let's merge this and I can release version 2.0 🎉

Ana06 · 2021-07-12T12:11:21Z

README.md

@@ -1,16 +1,18 @@
+# get-changed-files


That's awesome, thanks!

Ana06 · 2021-07-12T12:16:48Z

README.md

@@ -22,16 +24,23 @@ See [action.yml](action.yml)
    # Default: 'space-delimited'
    format: ''
    # Filter files using a regex
-    filter: '*'
+    glob-filter: '*'


should we call it grob-filter or just filter? I like filter, as we don't have any other filer I think the name doesn't need to be more specific as long as we document it properly. But I don't have a strong opinion... 🤔 🤔 Any other opinion?

README.md

src/main.ts

Ana06 · 2021-07-12T12:32:35Z

README.md


-## Get all changed files as space-delimited
+### Get all changed files as space-delimited


I know this was already like this before, but I think it would be clearer to have this Get all changed files as space-delimited without the filter option and make another section to only get *.php and give there some more details about how the filter option works (that a list of regular expression is expected, GitHub like syntax is used, etc.). Ideally users should be able to understand how it works only with the README without checking the code. I am not completely sure this is the case now.

Added one example

Ana06 · 2021-07-12T12:40:13Z

README.md

@@ -22,16 +24,23 @@ See [action.yml](action.yml)
    # Default: 'space-delimited'
    format: ''
    # Filter files using a regex
-    filter: '*'
+    glob-filter: '*'


@rnsc

should we also change the repository URLs in the package.json? Or are we planning on trying to merge in the original repo in the future?

I think we can change the urls. I was hopping upstream integrates these changes when I originally forked it, but it doesn't look like it will happen.

src/main.ts

rnsc · 2021-07-22T15:09:38Z

Hello @Ana06 , I think I have made all the changes required, what would be the next steps? Not sure if can help still!

Ana06

Great job @rnsc and sorry for the late response. I think it is ready to merge 🎉

[Feature] include/exclude filters (#1)

There as a discussion on the name to use in #2 and we forgot to change these.

Ana06 · 2021-07-27T13:54:18Z

.github/workflows/test.yml

@@ -85,12 +85,37 @@ jobs:
      - id: files
        name: Run the action
        uses: ./
+        with:
+          glob-filter: '*'


I had skipped that one 🙈 I have fix it on main 😉

Oh no! Sorry!

micalevisk approved these changes Jun 28, 2021

View reviewed changes

micalevisk approved these changes Jun 29, 2021

View reviewed changes

agraul mentioned this pull request Jun 29, 2021

[Feature] Filter : include/exclude #1

Closed

Ana06 reviewed Jun 30, 2021

View reviewed changes

rnsc added 4 commits July 1, 2021 22:04

Glob filtering (#2)

956182e

Support for glob filtering with multi-rules.

Update test.yml

0b98d12

Update test.yml

d371995

fix identation on package.json

de1f38c

rnsc commented Jul 1, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

rnsc commented Jul 1, 2021

View reviewed changes

micalevisk reviewed Jul 1, 2021

View reviewed changes

src/main.ts Outdated Show resolved Hide resolved

rnsc added 4 commits July 2, 2021 12:45

removed useless debug

eb061c4

updated package version

6efbdb0

updated README with version bump

0c6352b

add missing input in action.yml

e3da65b

micalevisk suggested changes Jul 2, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

adding carret to minimatch pkg ver

c1e82c4

micalevisk approved these changes Jul 2, 2021

View reviewed changes

rnsc requested a review from Ana06 July 6, 2021 10:49

rnsc commented Jul 6, 2021

View reviewed changes

Ana06 reviewed Jul 12, 2021

View reviewed changes

micalevisk suggested changes Jul 13, 2021

View reviewed changes

src/main.ts Outdated Show resolved Hide resolved

rnsc force-pushed the main branch 2 times, most recently from b05c075 to 068f1f4 Compare July 13, 2021 17:05

rnsc added 3 commits July 13, 2021 19:09

switch glob-filter param to filter

de73d10

pattern startsWith instead of includes

3269ec7

fix the README

873b7ea

rnsc force-pushed the main branch from 068f1f4 to 873b7ea Compare July 13, 2021 17:09

typo fix + yarn all

58c3e15

rnsc force-pushed the main branch from f1b3270 to 58c3e15 Compare July 13, 2021 17:12

micalevisk approved these changes Jul 22, 2021

View reviewed changes

Ana06 approved these changes Jul 27, 2021

View reviewed changes

Ana06 merged commit f0220ec into Ana06:main Jul 27, 2021

Ana06 pushed a commit that referenced this pull request Jul 27, 2021

Merge pull request #2 from rnsc/main

48961ca

[Feature] include/exclude filters (#1)

Ana06 added a commit that referenced this pull request Jul 27, 2021

Replace glob-filter by filter in test workflow

42676e4

There as a discussion on the name to use in #2 and we forgot to change these.

Ana06 reviewed Jul 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] include/exclude filters (#1) #2

[Feature] include/exclude filters (#1) #2

rnsc commented Jun 28, 2021

micalevisk left a comment

rnsc commented Jun 30, 2021

micalevisk commented Jun 30, 2021 •

edited

Loading

Ana06 left a comment

Ana06 Jun 30, 2021

rnsc Jun 30, 2021

rnsc commented Jun 30, 2021

micalevisk commented Jun 30, 2021

agraul commented Jul 1, 2021 •

edited

Loading

rnsc commented Jul 1, 2021

rnsc Jul 1, 2021

Ana06 Jul 12, 2021

rnsc Jul 1, 2021

micalevisk Jul 1, 2021

rnsc Jul 2, 2021

rnsc Jul 2, 2021

Ana06 Jul 12, 2021

Ana06 Jul 12, 2021

rnsc Jul 1, 2021

rnsc Jul 6, 2021

agraul Jul 14, 2021

rnsc commented Jul 12, 2021

Ana06 left a comment

Ana06 Jul 12, 2021

Ana06 Jul 12, 2021

Ana06 Jul 12, 2021

rnsc Jul 13, 2021

Ana06 Jul 12, 2021

rnsc commented Jul 22, 2021

Ana06 left a comment

Ana06 Jul 27, 2021

rnsc Jul 27, 2021


		## Get all changed files as space-delimited
		### Get all changed files as space-delimited

[Feature] include/exclude filters (#1) #2

[Feature] include/exclude filters (#1) #2

Conversation

rnsc commented Jun 28, 2021

micalevisk left a comment

Choose a reason for hiding this comment

rnsc commented Jun 30, 2021

micalevisk commented Jun 30, 2021 • edited Loading

Ana06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnsc commented Jun 30, 2021

micalevisk commented Jun 30, 2021

agraul commented Jul 1, 2021 • edited Loading

rnsc commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnsc commented Jul 12, 2021

Ana06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnsc commented Jul 22, 2021

Ana06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micalevisk commented Jun 30, 2021 •

edited

Loading

agraul commented Jul 1, 2021 •

edited

Loading